fix(multichain-account-service): stop BaseBip44AccountProvider leaking removed accounts and stale account IDs - #10069

What it says on the box

BaseBip44AccountProvider has two related bugs that both stem from the same root cause: its internal account-ID tracking (this.accounts) can silently drift from what the AccountsController actually knows about.

1. getAccounts() returns undefined entries cast as if they were real accounts

AccountsController.getAccounts(accountIds) is documented as returning (InternalAccount | undefined)[] — one slot per requested ID, undefined for any ID it doesn't recognize (e.g. the account was removed). BaseBip44AccountProvider#getAccounts() cast this straight through:

const internalAccounts = this.messenger.call('AccountsController:getAccounts', accountsIds);
return internalAccounts as unknown as Account[]; // could contain `undefined`

The sibling MultichainAccountGroup#getAccounts() already guards this exact case (if (account) { ... } // might mean it has been deleted), but BaseBip44AccountProvider's own batch method didn't.

2. init() is purely additive, so a re-init after a removal leaves stale IDs forever

init(accounts: Account['id'][]): void {
  for (const account of accounts) {
    this.accounts.add(account);
  }
}

MultichainAccountService#init() calls provider.init(state) on every provider, computed fresh from current account state, every time it runs. It's not a one-shot boot call — metamask-mobile's Authentication.ts calls MultichainAccountService.init() on every unlock:

// app/core/Authentication/Authentication.ts
const { MultichainAccountService } = Engine.context;
await MultichainAccountService.init();

So: remove an account, lock, unlock — the provider re-inits with a fresh (smaller) list, but the old ID is still in this.accounts because init() only ever adds.

Receipts

Real regression tests added (BaseBip44AccountProvider.test.ts), using a mock AccountsController:getAccounts handler that mirrors the real .map()-based contract (same-length array, undefined in place for missing IDs — not a shorter, filtered array, which is what happened to make this invisible to EvmAccountProvider.test.ts's existing mock).

Genuine red-before/green-after via git stash on the source fix alone:

$ git stash push -- .../BaseBip44AccountProvider.ts
$ yarn jest BaseBip44AccountProvider.test.ts
Tests: 3 failed, 2 passed, 5 total
$ git stash pop
$ yarn jest BaseBip44AccountProvider.test.ts
Tests: 5 passed, 5 total

Full package suite: Test Suites: 17 passed, 17 total / Tests: 353 passed, 353 total, 100% statement/function/line coverage, 96.85% branch (unchanged from baseline).

yarn lint:tsc (the real monorepo-wide tsc --build, not a scoped package check, which silently no-ops for this package since it has no local lint:tsc script) and yarn eslint on all changed files both clean.

Also updated the shared tests/providers.ts mock provider's init() implementation to match the real replace-not-add semantics — it's used by higher-level MultichainAccountService/MultichainAccountWallet tests, and was otherwise silently modeling the old buggy behavior even after this fix. Full suite re-confirmed green after that change too (nothing relied on the old mock behavior).

Fix

• Filter undefined entries out of getAccounts() instead of casting them through.
• init() now does this.accounts = new Set(accounts) — replaces rather than adds.

Review

Reviewed adversarially with Codex (codex exec, synchronous). It found no production correctness bug in the intended fix, but caught two real issues in my first pass, both fixed:

• The new test imported KeyringCapabilities from the wrong subpath (@metamask/keyring-api instead of @metamask/keyring-api/v2) — a real TS2305, only visible under the monorepo's real tsc --build, not the scoped package check I'd run first.
• The shared tests/providers.ts mock's init() was still additive, inconsistent with the fix (described above, fixed).